Skip to content

fix: stop one transient failure from permanently killing an event consumer - #64

Merged
HandyS11 merged 3 commits into
developfrom
fix/consumer-loop-resilience
Jul 24, 2026
Merged

fix: stop one transient failure from permanently killing an event consumer#64
HandyS11 merged 3 commits into
developfrom
fix/consumer-loop-resilience

Conversation

@HandyS11

Copy link
Copy Markdown
Owner

Why

Three faults observed live tonight, all the same class: an exception escaping into a consumer's await foreach ends that subscription for the rest of the process, so the feature it drives goes silently dead until the bot restarts. The outer try/catch around each loop reads as defensive, but it logs once and exits.

Same failure class as #63, which fixed it for one connection loop only.

1. Map connection-status loop faulted. InvalidOperationException: GetMap returned no data.

ConnectionSupervisor.GetMonumentsAsync forwarded the connection-level call (documented "Throws on failure") straight through a query seam whose own contract promised "or an empty list". It was the only query in RustPlusSocketSource without a degrade-to-null/empty catch, so a transient Rust+ error reached MapComposer and killed the map's status loop — no repaint on connect, no cache clear on disconnect, for the rest of the run.

It now catches, logs and returns [], and the failure message names the Rust+ error code (rate_limit, no_map, …) instead of a bare "returned no data".

2. ConnectionStatusChanged consumer faulted. TimeoutException (Discord REST)

A routine RequestBucket.EnterAsync timeout during a workspace reconcile killed the Workspace consumer, leaving the info channels stale. The code documented this as acceptable ("the reconciler is idempotent and a restart heals") — but a Discord timeout is ordinary traffic, not an exceptional event.

3. The same shape existed, unguarded, in 21 consumer loops across 11 services

Their relay handlers post to Discord and had no internal try/catch, so one REST timeout would permanently silence that feature.

What

New EventBusConsumption.ConsumeAsync<TEvent>(handler, onHandlerFailure, ct): subscribes, guards each handler call, reports failures and keeps consuming. Only cancellation of the loop token ends the subscription. RustPlusBot.Abstractions stays dependency-free — the callback is Action<Exception>, so each service keeps its own logger.

Every vulnerable consumer routes through it — Workspace (4), Alarms (6), Switches (6), StorageMonitors (5), Events (3), Map (3), Chat (2), Connections (2), Wipes (2), Players (1), InfoMap (1). Each service's outer try/catch stays as a backstop for a genuinely broken subscription.

ClansHostedService and CommandsHostedService are left alone deliberately: they already isolate per event inline, and Clans' failure log carries guild/server ids the shared callback can't reach.

Also

Rust+ sends the token apartmentcomplex, which no entry in MonumentTokenMap mapped, so apartment complexes rendered with no icon (No monument icon for token "apartmentcomplex" (mapped type: null)). MonumentType.ApartmentsComplex and Apartments_Complex.svg were already in RustMapsApi.Assets — only the token table hadn't caught up.

Tests

  • EventBusConsumptionTests — a failing handler costs its own event only; cancellation still ends the subscription.
  • WorkspaceConsumerResilienceTests and MapHostedServiceTests — both written failing first, reproducing the live exceptions (calls: 1 before the fix).
  • A monuments-seam case in ServerQueryTests (failed with the exact production exception) and an apartmentcomplex case in MonumentTokenMapTests.

dtk dtk dotnet test RustPlusBot.slnx1253 passed, 1 skipped, 19 projects.

Not addressed

Why Discord's request bucket timed out, and why GetMap failed 3s after connect (three heavy GetMap round-trips land back-to-back on the connect path — plausible rate-limiting). This PR makes both survivable and self-describing; the new log lines will name the cause next time.

🤖 Generated with Claude Code

Three faults observed live tonight, all the same class: an exception escaping
into a consumer's await-foreach ends that subscription for the rest of the
process, so the feature it drives goes silently dead until the bot restarts.

1. "Map connection-status loop faulted. InvalidOperationException: GetMap
   returned no data." — ConnectionSupervisor.GetMonumentsAsync forwarded the
   connection-level call (documented "Throws on failure") straight through a
   query seam whose own contract promised "or an empty list". It was the sole
   query in RustPlusSocketSource without a degrade-to-null/empty catch. It now
   catches, logs and returns [], and the failure message names the Rust+ error
   code instead of a bare "returned no data".

2. "ConnectionStatusChanged consumer faulted. TimeoutException" from a Discord
   REST edit — the Workspace consumer died, leaving the info channels stale.

3. The same shape existed, unguarded, in 21 consumer loops across 11 services.

Add EventBusConsumption.ConsumeAsync<TEvent>(handler, onHandlerFailure, ct):
it subscribes, guards each handler call, reports failures and keeps consuming;
only cancellation of the loop token ends the subscription. Abstractions stays
dependency-free (the callback is Action<Exception>, so each service keeps its
own logger). Every vulnerable consumer now routes through it; each service's
outer try/catch stays as a backstop. ClansHostedService and
CommandsHostedService already isolate per event inline and are left alone —
migrating them would lose the guild/server ids in their failure logs.

Also fixes the unrelated log line that started this: Rust+ sends the token
"apartmentcomplex", which no entry in MonumentTokenMap mapped, so apartment
complexes rendered with no icon ("No monument icon for token ...; mapped type:
null"). MonumentType.ApartmentsComplex and Apartments_Complex.svg were already
in RustMapsApi.Assets — only the token table had not caught up.

Tests: EventBusConsumptionTests (a failing handler costs its own event only;
cancellation still ends the subscription), WorkspaceConsumerResilienceTests and
MapHostedServiceTests (both written failing first, reproducing the live
exceptions), plus a monuments-seam and a token-map case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 24, 2026 02:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the bot’s long-running event consumers so a single transient exception in a handler (e.g., Discord REST timeouts, Rust+ endpoint hiccups) no longer permanently terminates an await foreach subscription and silently disables a feature until restart. It introduces a shared consumption helper and routes many hosted-service consumer loops through it, plus adds targeted regression tests and fixes a missing monument token mapping.

Changes:

  • Add EventBusConsumption.ConsumeAsync<TEvent>(...) to catch per-handler exceptions, report them, and continue consuming until cancellation.
  • Update multiple hosted services to consume events via ConsumeAsync (and add guarded periodic map refresh work).
  • Add/extend tests for consumer resilience and for monument query/token mapping behavior; improve Rust+ “GetMap returned no data” error detail and degrade monuments queries to [].

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/RustPlusBot.Abstractions/Events/EventBusConsumption.cs Adds a resilient event-consumption helper that isolates handler failures.
src/RustPlusBot.Features.Workspace/Hosting/WorkspaceHostedService.cs Switches workspace consumers to resilient consumption to avoid permanent subscription death.
src/RustPlusBot.Features.Map/Hosting/MapHostedService.cs Uses resilient consumption for map event loops and guards periodic refresh work units.
src/RustPlusBot.Features.Map/Hosting/InfoMapHostedService.cs Uses resilient consumption for connection-status driven info-map behavior.
src/RustPlusBot.Features.Connections/Hosting/ConnectionHostedService.cs Uses resilient consumption for connection supervisor event handling.
src/RustPlusBot.Features.Connections/Supervisor/ConnectionSupervisor.cs Degrades monuments fetch failures to [] to protect downstream consumers.
src/RustPlusBot.Features.Connections/Listening/RustPlusSocketSource.cs Improves “GetMap returned no data” exception message with Rust+ error details.
src/RustPlusBot.Features.Chat/Hosting/ChatHostedService.cs Routes chat relay consumers through resilient consumption; keeps name-record isolation.
src/RustPlusBot.Features.Events/Hosting/EventsHostedService.cs Routes events relays through resilient consumption.
src/RustPlusBot.Features.Wipes/Hosting/WipesHostedService.cs Routes wipe detection/announcement consumers through resilient consumption.
src/RustPlusBot.Features.Switches/Hosting/SwitchesHostedService.cs Routes multiple switch consumers through resilient consumption.
src/RustPlusBot.Features.StorageMonitors/Hosting/StorageMonitorsHostedService.cs Routes storage monitor consumers through resilient consumption.
src/RustPlusBot.Features.Alarms/Hosting/AlarmsHostedService.cs Routes alarm consumers through resilient consumption.
src/RustPlusBot.Features.Players/Hosting/PlayersHostedService.cs Routes player relay consumer through resilient consumption.
src/RustPlusBot.Features.Map/Assets/MonumentTokenMap.cs Adds missing apartmentcomplex token mapping.
src/RustPlusBot.Abstractions/Connections/IRustServerQuery.cs Clarifies monuments query contract: never throws for fetch failure; returns empty list.
tests/RustPlusBot.Abstractions.Tests/Events/EventBusConsumptionTests.cs Adds tests proving handler failures don’t end subscription; cancellation still ends it.
tests/RustPlusBot.Features.Workspace.Tests/Hosting/WorkspaceConsumerResilienceTests.cs Adds regression test for workspace consumer surviving a faulting reconcile.
tests/RustPlusBot.Features.Map.Tests/Hosting/MapHostedServiceTests.cs Adds regression test for map status loop surviving a faulting refresh.
tests/RustPlusBot.Features.Connections.Tests/ServerQueryTests.cs Adds test ensuring monuments query degrades to empty on endpoint failure.
tests/RustPlusBot.Features.Connections.Tests/Fakes/FakeRustSocketSource.cs Extends fake to simulate monuments endpoint faults.
tests/RustPlusBot.Features.Map.Tests/MonumentTokenMapTests.cs Adds coverage for apartmentcomplex token mapping.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +323 to +332
#pragma warning disable CA1031 // Broad catch: this seam promises degradation, so a failed fetch is "no monuments".
catch (Exception ex)
#pragma warning restore CA1031
{
// The connection-level call throws on a failed/timed-out GetMap (rate limit, no map, slow
// endpoint). Callers here are render paths that must degrade to an icon-less map, never fault:
// an escaping exception tears down the consuming loop for the rest of the process.
LogMonumentsFetchFailed(logger, ex, serverId);
return [];
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in 60454c9. The query seam now logs a generic LogMonumentsQueryFailed ("Querying monuments for server {ServerId} failed; returning no monuments for this call."); the rig-specific message stays on the rig-detection path.

HandyS11 and others added 2 commits July 24, 2026 11:28
Copilot review: GetMonumentsAsync (the query seam used by map rendering) reused
LogMonumentsFetchFailed, whose message says "rig detection disabled for this
connection" — misleading for a non-rig caller. Give the seam its own generic
LogMonumentsQueryFailed and keep the rig-specific message on the rig path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AdvancingClock jumped 1 minute per read while MapRefreshInterval is 30 minutes,
so MapRefreshThrottle gated every refresh after the first: the counter only
re-incremented once ~30 clock reads accumulated. Fast enough locally, but on
Windows CI fewer than 30 events were consumed inside the 5s deadline, so calls
stuck at 1 and the assert failed. Advance an hour per read (past the interval)
so the throttle passes every refresh and each event reaches the counter.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@HandyS11
HandyS11 merged commit a1e700c into develop Jul 24, 2026
3 checks passed
@HandyS11
HandyS11 deleted the fix/consumer-loop-resilience branch July 24, 2026 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants